fix(rich-text): link editor selection highlight and prepopulate URL #417
Conversation
|
✅ Deploy Preview for stacks-editor ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
alizaberger
requested review from
alexwarren,
dancormier,
giamir and
threefjefff
dancormier
left a comment
dancormier
left a comment
There was a problem hiding this comment.
I've taken a look at the code and tested the changes out. This is a nice improvement. The sort of little annoyance I've experienced dozens of times. Thanks for tackling this!
I only had one minor UI suggestion but I wouldn't let it hold up merging if you and/or others feel differently
src/rich-text/commands/index.ts
Outdated
| null; | ||
| const linkMatch = /^http(s)?:\/\/\S+$/.exec(selectedText); | ||
| linkUrl = linkMatch?.length > 0 ? linkMatch[0] : ""; | ||
| linkUrl = linkMatch?.length > 0 ? linkMatch[0] : "https://"; |
There was a problem hiding this comment.
I'd find this a little less convenient because I'd paste a link here most often, so having the input prepopulated with https:// adds a step of deleting the text. Instead, my suggestion is to use the placeholder attribute with a value of https://… or something like "Add a link" similar to Google Docs:
I don't think the placeholder is entirely necessary as long as the Link URL field is focused upon invoking the link editor.
All this said, I'd like to get others' opinions on this since it's entirely possible this is just a me thing.
There was a problem hiding this comment.
I agree with @dancormier. I don't think it is a good practice to prefill text input. Placeholders are a more conventional way of giving hints to users. I would recommend to check with your designer about this whole flow and seek their input. 🙂
giamir
left a comment
giamir
left a comment
There was a problem hiding this comment.
Thanks, @alizaberger, for the PR! I noticed a small regression at the end of the flow—after the user clicks the save button, the popover with the link appears again, but the arrow is no longer pointing to the center of the link. See screenshots.
Before (see arrow centered)
After (see arrow not centered anymore)
I know @alexwarren did some work on links/popovers a while back, so you might want to rebase this branch. That might be all that’s needed to make the issue go away.
Apart from that, only minor things. Thanks for making the editor better. ❤️
src/rich-text/plugins/link-editor.ts
Outdated
| this.hrefInput.value = state.url; | ||
| this.validate(state.url); | ||
|
|
||
| if (state.url != "https://") { |
There was a problem hiding this comment.
NIT: Consider using identity (===) instead of equality (==) here. In JavaScript, == allows type coercion, which can lead to unexpected behavior—for example, 0 == '0' is true, but 0 === '0' is false. Using identity (===) ensures both the value and the type match, which tends to be safer and more predictable.
99.9% of the time in JS we should use identity (===) to avoid surprises. Quirks of an old language written in 10 days. 🙂
src/rich-text/commands/index.ts
Outdated
| null; | ||
| const linkMatch = /^http(s)?:\/\/\S+$/.exec(selectedText); | ||
| linkUrl = linkMatch?.length > 0 ? linkMatch[0] : ""; | ||
| linkUrl = linkMatch?.length > 0 ? linkMatch[0] : "https://"; |
There was a problem hiding this comment.
I agree with @dancormier. I don't think it is a good practice to prefill text input. Placeholders are a more conventional way of giving hints to users. I would recommend to check with your designer about this whole flow and seek their input. 🙂
|
Whoops, did not realize auto-merge would work without an approval! Sorry! |
|
No problem, thanks for addressing the PR comments. 🙂 |
Describe your changes
This PR makes inserting links less confusing in the rich text view.
https://PR Checklist
/** ... */docsbug/enhancementand other labels as appropriateEnvironment(s) tested
Additional context
Add any other context about the PR here.